Skip to content

Conversation

@realzachbrownjohn
Copy link
Contributor

@realzachbrownjohn realzachbrownjohn commented Oct 21, 2025

This is my first contribution so I would appreciate your guidance. I think I have fixed the issue, but I'm confused on where to add test(s), or if I need to at all. Would it be acceptable to use Dr-Irv's reproduction script, or perhaps a modified copy?

Copy link
Contributor

@cmp0xff cmp0xff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please look for test_multiindex to place the tests. I think you need to add one or more new test functions in tests/indexes/test_indexes.py.

@realzachbrownjohn
Copy link
Contributor Author

Thank you for the feedback! I will work on that shortly.

@realzachbrownjohn
Copy link
Contributor Author

realzachbrownjohn commented Oct 22, 2025

I've tried my best to spend a bit of time unpacking this. Ultimately, I didn't think it was best to include all types of Index in the overload because not all types of Index are valid to union with MultiIndex (only those with tuples). Referring to tuples within Indexes wasn't supported so I had to add that support in _typing.pyi.

Happy to adjust recognising that Dr-Irv never mentioned any issue with using the complete Index type.

Additionally, I've attempted to add a test in tests/indexes/test_indexes.py covering the different cases I could think of.

@realzachbrownjohn
Copy link
Contributor Author

I can see now that all checks have failed, looks related to my extension of S1 (in adding support for tuple[Hashable, ...] I seem to have violated some sort of constraint). Any guidance would be greatly appreciated.

Copy link
Contributor

@cmp0xff cmp0xff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • You can mark the PR as a draft if you plan to work on it for a while.
    • Just click Re-request review on the top-right corner of the PR when you are done.
  • Please run pre-commit locally to avoid simple mistakes
  • Please use poe to do further tests locally, e.g. poe test_all

@realzachbrownjohn realzachbrownjohn marked this pull request as draft October 22, 2025 13:50
@realzachbrownjohn realzachbrownjohn marked this pull request as ready for review October 22, 2025 14:08
@realzachbrownjohn
Copy link
Contributor Author

Ended up removing MultiIndex-Index unions from assumption for returning a MultiIndex - was causing too many problems and Dr-Irv said it was okay to ignore. It was either violating the overload standard or confusing mypy in testing.

swaplevel tests added, MultiIndex tests removed some cases relating to different types of Indexes (since we no longer try checking for those), and S1 changes reverted.

@realzachbrownjohn
Copy link
Contributor Author

realzachbrownjohn commented Oct 22, 2025

I'm not sure why it failed. I ran poe test_all locally several times; all successful.

Running the test case locally individually it failed, seems to be a runtime error because Pandas won't accept an Index unionising a MultiIndex (as it should). Removing that test case.

Copy link
Contributor

@cmp0xff cmp0xff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also run pre-commit locally.

@realzachbrownjohn realzachbrownjohn marked this pull request as draft October 22, 2025 22:15
@realzachbrownjohn
Copy link
Contributor Author

Please also run pre-commit locally.

My mistake. I had ran a pre-commit after committing, and didn't commit the refactoring it made.

@realzachbrownjohn realzachbrownjohn marked this pull request as ready for review October 22, 2025 22:34
Copy link
Contributor

@cmp0xff cmp0xff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, now all checks pass. We are almost there.

Co-authored-by: Yi-Fan Wang <cmp0xff@users.noreply.github.com>
@realzachbrownjohn realzachbrownjohn marked this pull request as draft October 23, 2025 22:25
@realzachbrownjohn realzachbrownjohn marked this pull request as ready for review October 23, 2025 22:35
@realzachbrownjohn realzachbrownjohn marked this pull request as draft October 23, 2025 22:50
@realzachbrownjohn realzachbrownjohn marked this pull request as ready for review October 23, 2025 22:54
Copy link
Contributor

@cmp0xff cmp0xff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two small things.

  • In addition please also fix mypy type checking and further poe checks.

Co-authored-by: Yi-Fan Wang <cmp0xff@users.noreply.github.com>
@realzachbrownjohn realzachbrownjohn marked this pull request as draft October 24, 2025 09:23
@realzachbrownjohn
Copy link
Contributor Author

  • In addition please also fix mypy type checking and further poe checks.

Removing the overload of union from the MultiIndex class and instead setting MultiIndex to return Self caused a few existing tests to fail. I would prefer to reinstate the overload to maintain the existing test behaviour, but I could alternatively just modify the existing tests.

Which would you prefer?

@cmp0xff
Copy link
Contributor

cmp0xff commented Oct 24, 2025

I believe your changes actually improve the typing in the exising tests, if you take a look. So you can update the static and runtime typing checks in the tests.

In the stubs file for RangeIndex, you can try to ignore the error, or use @override.

@realzachbrownjohn
Copy link
Contributor Author

realzachbrownjohn commented Oct 24, 2025

In the stubs file for RangeIndex, you can try to ignore the error, or use @override.

@override did not work unfortunately - how do I ignore the error; is this even permitted?

Edit: Nevermind, figured out how to ignore the error.

@realzachbrownjohn realzachbrownjohn marked this pull request as ready for review October 24, 2025 09:51
@realzachbrownjohn
Copy link
Contributor Author

realzachbrownjohn commented Oct 24, 2025

I don't understand. poe test_all passes on my machine. Does it not cover the same test suite? The errors appear to be runtime-based.

Copy link
Contributor

@cmp0xff cmp0xff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @realzachbrownjohn ! I hope you enjoyed your first PR.

@cmp0xff cmp0xff merged commit 143bab4 into pandas-dev:main Oct 24, 2025
13 checks passed
@realzachbrownjohn
Copy link
Contributor Author

Thank you @realzachbrownjohn ! I hope you enjoyed your first PR.

Thank you for your help! I did!

@realzachbrownjohn realzachbrownjohn deleted the bugfix branch October 24, 2025 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MultiIndex method corrections

3 participants